-
Notifications
You must be signed in to change notification settings - Fork 477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: blocks/{round}/logs endpoint #5865
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5865 +/- ##
==========================================
+ Coverage 52.42% 56.17% +3.75%
==========================================
Files 482 482
Lines 67969 68009 +40
==========================================
+ Hits 35630 38205 +2575
+ Misses 29656 27210 -2446
+ Partials 2683 2594 -89 ☔ View full report in Codecov by Sentry. |
I've verified the logs are returned properly via external testing here: https://github.com/joe-p/block-log-debug/blob/6daea7ad53b83366c75aaf3325c719bc6fbd0646/__test__/block-log-debug.test.ts#L12 I'll need to write the proper tests here but should be good to go. Other things to consider:
|
Since this returns, at most, the logs in one block, I don't think you need to add any filtering or limiting. |
Yeah true, we know the upper bound due to the block size limit. Keeping it as is is good with me then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small things you may or may not bother with.
Co-authored-by: John Jannotti <jannotti@gmail.com>
Co-authored-by: John Jannotti <jannotti@gmail.com>
5971300
to
8c3ae7e
Compare
@jasonpaulos @jannotti I just added a fairly basic test in |
Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>
Co-authored-by: John Jannotti <jannotti@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy if we get some form of the description change I recommended. I want the response order to be defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to being more explicit about the endpoint description, but other than that, this looks good
Co-authored-by: John Jannotti <john.jannotti@algorand.com>
closes #5807 |
Summary
This is a new endpoint to get all of the logs from all appcalls in a given block. This endpoint was created specifically due to the encoding issue with the current
/blocks/{round}
endpoint (#5807).Test Plan
Tests cover the following scenarios: